Destinations V2: move create raw tables earlier #28255
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
octavia-squidington-iii
added
area/connectors
Before Merging a Connector Pull RequestWow! What a great pull request you have here! 🎉 To merge this PR, ensure the following has been done/considered for each connector added or updated:
If the checklist is complete, but the CI check is failing,
|
This comment was marked as outdated.
This comment was marked as outdated.
evantahler
approved these changes
Jul 12, 2023
|
A philosophical question for the Destinations team: Should branching logic like this be in Java or SQL? e.g. we first do a read to see if a table exists, and then if it doesn't exist, we create the table. Alternatively, we could always issue a Which way is better and why? |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
| Step | Result |
|---|---|
| Validate airbyte-integrations/connectors/destination-bigquery-denormalized/metadata.yaml | ✅ |
| Connector version semver check | ✅ |
| QA checks | ✅ |
| Build connector tar | ✅ |
| Build destination-bigquery-denormalized docker image for platform linux/x86_64 | ✅ |
| ./gradlew :airbyte-integrations:connectors:destination-bigquery-denormalized:integrationTest | ✅ |
Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command
airbyte-ci connectors --name=destination-bigquery-denormalized test
|
| Step | Result |
|---|---|
| Validate airbyte-integrations/connectors/destination-bigquery/metadata.yaml | ✅ |
| Connector version semver check | ✅ |
| Connector version increment check | ✅ |
| QA checks | ✅ |
| Build connector tar | ✅ |
| Build destination-bigquery docker image for platform linux/x86_64 | ✅ |
| Build airbyte/normalization:dev | ✅ |
| ./gradlew :airbyte-integrations:connectors:destination-bigquery:integrationTest | ❌ |
Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command
airbyte-ci connectors --name=destination-bigquery test
edgao
approved these changes
Jul 13, 2023
There was a problem hiding this comment.
I think java is basically unavoidable - e.g. once we start building migration stuff, we'll (probably?) want more complex logic like if (!findExistingTable().hasColumn("_airbyte_raw_id")) { doSomething() }. That would be pretty annoying in pure sql
e.g. BigQuerySqlGenerator.alterTable, which depends on us querying the db + use java to generate a sql statement
though... whether we prefer bigquery.createTable vs bigquery.execute("create table...") is also a question 🤔
|
The failing test was: I ran it locally and it was fine so I'm going to merge. |
octavia-approvington
added a commit
that referenced
this pull request
Jul 14, 2023
* Revert "Revert "Destination Bigquery: Scaffolding for destinations v2 (#27268)"" This reverts commit 348c577. * version bumps+changelog * Speed up BQ by having 2 queries, and not an OR (#27981) * 🐛 Destination Bigquery: fix bug in standard inserts for syncs >10K records (#27856) * only run t+d code if it's enabled * dockerfile+changelog * remove changelog entry * Destinations V2: handle optional fields for `object` and `array` types (#27898) * catch null schema * fix null properties * clean up * consolidate + add more tests * try catch * empty json test * Automated Commit - Formatting Changes * remove todo * destination bigquery: misc updates to 1s1t code (#28057) * switch to checkedconsumer * add unit test for buildColumnId * use flag * restructure prefix check * fix build * more type-parsing fixes (#28100) * more type-parsing fixes * handle duplicates * Automated Commit - Format and Process Resources Changes * add tests for asColumns * Automated Commit - Format and Process Resources Changes * log warnings instead of throwing exception * better log message * error level --------- Co-authored-by: edgao <edgao@users.noreply.github.com> * Automated Commit - Formatting Changes * Improve protocol type parsing (#28126) * Automated Commit - Formatting Changes * Change from T&D every 10k records to an increasing time based interval (#28130) * fifteen minute t&d * add typing and deduping operation valve for increased intervals of typing and deduping * Automated Commit - Format and Process Resources Changes * resolve bizarre merge conflict * Automated Commit - Format and Process Resources Changes --------- Co-authored-by: jbfbell <jbfbell@users.noreply.github.com> * Simplify and speed up CDC delete support [DestinationsV2] (#28029) * Simplify and speed up CDC delete support [DestinationsV2] * better QUOTE * spotbugs? * recompile dbt image for local arch and use that when building images * things compile, but tests fail * tests working-ish * comment * fix logic to re-insert deleted records for cursor comparison. tests pass! * remove comment * Skip CDC re-include logic if there are no CDC columns * stop hardcoding pk (#28092) * wip * remove TODOs --------- Co-authored-by: Edward Gao <edward.gao@airbyte.io> * update method name * Automated Commit - Formatting Changes * depend on pinned normalization version * implement 1s1t DATs for destination-bigquery (#27852) * intiial implementation * Automated Commit - Formatting Changes * add second sync to test * do concurrent things * Automated Commit - Formatting Changes * clarify comment * minor tweaks * more stuff * Automated Commit - Formatting Changes * minor cleanup * lots of fixes * handle sql vs json null better * verify extra columns * only check deleted_at if in DEDUP mode and the column exists * add full refresh append test case * Automated Commit - Formatting Changes * add tests for the remaining sync modes * Automated Commit - Formatting Changes * readability stuff * Automated Commit - Formatting Changes * add test for gcs mode * remove static fields * Automated Commit - Formatting Changes * add more test cases, tweak test scaffold * cleanup * Automated Commit - Formatting Changes * extract recorddiffer * and use it in the sql generator test * fix * comment * naming+comment * one more comment * better assert * remove unnecessary thing * one last thing * Automated Commit - Formatting Changes * enable concurrent execution on all java integration tests * add test for default namespace * Automated Commit - Formatting Changes * implement a 2-stream test * Automated Commit - Formatting Changes * extract methods * invert jsonNodesNotEquivalent * Automated Commit - Formatting Changes * fix conditional * pull out diffSingleRecord * Automated Commit - Formatting Changes * handle nulls correctly * remove raw-specific handling; break up methods * Automated Commit - Formatting Changes --------- Co-authored-by: edgao <edgao@users.noreply.github.com> Co-authored-by: octavia-approvington <octavia-approvington@users.noreply.github.com> * Destinations V2: move create raw tables earlier (#28255) * move create raw tables * better log message * stop building normalization (#28256) * fix ability to run tests * disable incremental t+d for now * Automated Commit - Formatting Changes --------- Co-authored-by: Evan Tahler <evan@airbyte.io> Co-authored-by: Cynthia Yin <cynthia@airbyte.io> Co-authored-by: cynthiaxyin <cynthiaxyin@users.noreply.github.com> Co-authored-by: edgao <edgao@users.noreply.github.com> Co-authored-by: Joe Bell <joseph.bell@airbyte.io> Co-authored-by: jbfbell <jbfbell@users.noreply.github.com> Co-authored-by: octavia-approvington <octavia-approvington@users.noreply.github.com>
efimmatytsin
pushed a commit
to scentbird/airbyte
that referenced
this pull request
Jul 27, 2023
* Revert "Revert "Destination Bigquery: Scaffolding for destinations v2 (airbytehq#27268)"" This reverts commit 348c577. * version bumps+changelog * Speed up BQ by having 2 queries, and not an OR (airbytehq#27981) * 🐛 Destination Bigquery: fix bug in standard inserts for syncs >10K records (airbytehq#27856) * only run t+d code if it's enabled * dockerfile+changelog * remove changelog entry * Destinations V2: handle optional fields for `object` and `array` types (airbytehq#27898) * catch null schema * fix null properties * clean up * consolidate + add more tests * try catch * empty json test * Automated Commit - Formatting Changes * remove todo * destination bigquery: misc updates to 1s1t code (airbytehq#28057) * switch to checkedconsumer * add unit test for buildColumnId * use flag * restructure prefix check * fix build * more type-parsing fixes (airbytehq#28100) * more type-parsing fixes * handle duplicates * Automated Commit - Format and Process Resources Changes * add tests for asColumns * Automated Commit - Format and Process Resources Changes * log warnings instead of throwing exception * better log message * error level --------- Co-authored-by: edgao <edgao@users.noreply.github.com> * Automated Commit - Formatting Changes * Improve protocol type parsing (airbytehq#28126) * Automated Commit - Formatting Changes * Change from T&D every 10k records to an increasing time based interval (airbytehq#28130) * fifteen minute t&d * add typing and deduping operation valve for increased intervals of typing and deduping * Automated Commit - Format and Process Resources Changes * resolve bizarre merge conflict * Automated Commit - Format and Process Resources Changes --------- Co-authored-by: jbfbell <jbfbell@users.noreply.github.com> * Simplify and speed up CDC delete support [DestinationsV2] (airbytehq#28029) * Simplify and speed up CDC delete support [DestinationsV2] * better QUOTE * spotbugs? * recompile dbt image for local arch and use that when building images * things compile, but tests fail * tests working-ish * comment * fix logic to re-insert deleted records for cursor comparison. tests pass! * remove comment * Skip CDC re-include logic if there are no CDC columns * stop hardcoding pk (airbytehq#28092) * wip * remove TODOs --------- Co-authored-by: Edward Gao <edward.gao@airbyte.io> * update method name * Automated Commit - Formatting Changes * depend on pinned normalization version * implement 1s1t DATs for destination-bigquery (airbytehq#27852) * intiial implementation * Automated Commit - Formatting Changes * add second sync to test * do concurrent things * Automated Commit - Formatting Changes * clarify comment * minor tweaks * more stuff * Automated Commit - Formatting Changes * minor cleanup * lots of fixes * handle sql vs json null better * verify extra columns * only check deleted_at if in DEDUP mode and the column exists * add full refresh append test case * Automated Commit - Formatting Changes * add tests for the remaining sync modes * Automated Commit - Formatting Changes * readability stuff * Automated Commit - Formatting Changes * add test for gcs mode * remove static fields * Automated Commit - Formatting Changes * add more test cases, tweak test scaffold * cleanup * Automated Commit - Formatting Changes * extract recorddiffer * and use it in the sql generator test * fix * comment * naming+comment * one more comment * better assert * remove unnecessary thing * one last thing * Automated Commit - Formatting Changes * enable concurrent execution on all java integration tests * add test for default namespace * Automated Commit - Formatting Changes * implement a 2-stream test * Automated Commit - Formatting Changes * extract methods * invert jsonNodesNotEquivalent * Automated Commit - Formatting Changes * fix conditional * pull out diffSingleRecord * Automated Commit - Formatting Changes * handle nulls correctly * remove raw-specific handling; break up methods * Automated Commit - Formatting Changes --------- Co-authored-by: edgao <edgao@users.noreply.github.com> Co-authored-by: octavia-approvington <octavia-approvington@users.noreply.github.com> * Destinations V2: move create raw tables earlier (airbytehq#28255) * move create raw tables * better log message * stop building normalization (airbytehq#28256) * fix ability to run tests * disable incremental t+d for now * Automated Commit - Formatting Changes --------- Co-authored-by: Evan Tahler <evan@airbyte.io> Co-authored-by: Cynthia Yin <cynthia@airbyte.io> Co-authored-by: cynthiaxyin <cynthiaxyin@users.noreply.github.com> Co-authored-by: edgao <edgao@users.noreply.github.com> Co-authored-by: Joe Bell <joseph.bell@airbyte.io> Co-authored-by: jbfbell <jbfbell@users.noreply.github.com> Co-authored-by: octavia-approvington <octavia-approvington@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #27921
BigQuery was complaining about raw tables not being found for sources with many rows (ex: source faker with 10M rows) when loaded via standard inserts. Moved raw table creation from consumer close to consumer startTracked.
Tested on localhost with 10M row faker source and shared test workspace.